fix(1456): forgot-password no longer leaks whether email is registered#1461
Merged
Conversation
Pre-fix `api_auth_lost_password` threw `ApiError('not_registered', …)`
on the miss branch and `ApiError('mail_failed', …)` on the SMTP-
failure branch, with the success path emitting a "Check E-Mail"
envelope. An unauthenticated visitor could enumerate every
registered admin email on the panel by posting one address per
request and reading the painted toast title back ("Check E-Mail"
→ registered; "Error" + "not registered" → unregistered).
Post-fix every reachable branch returns the same generic
"Check E-Mail" envelope ("If that email is registered, a link has
been sent — check your inbox and spam folder"). DB writes + SMTP
round-trip still gate on the matched branch so the panel never
becomes an open mail relay AND a hostile probe doesn't get to
amplify writes against `:prefix_admins.validate`. Operator-side
audit-log entry on the matched-branch SMTP failure path keeps the
"SMTP misconfigured" diagnostic visible without revealing per-
account state to the caller.
`config.enablenormallogin=0` still surfaces the `disabled` error
envelope: it's an operator toggle, not a per-user signal — the
value is the same for every caller.
Documented the contract under "Public auth surfaces:
response-shape uniformity" in AGENTS.md, with a matching
Anti-patterns entry and a "Where to find what" row pointing
future work at the reference shape. Sibling enumeration leaks on
`api_auth_login` (the `?m=…` flag branching) are flagged for a
follow-up.
Caveat: the response-time differential between the matched (SMTP
round-trip) and missed (immediate return) branches remains. A
determined attacker can still enumerate via timing; closing that
requires a background-worker queued send or a deliberate pad-the-
miss approach. Both are out of scope here. The user-visible
envelope-shape leak (which the reporter saw) is closed.
Regression guards:
- `web/tests/api/AuthTest.php::testLostPasswordResponseIsIdenticalForKnownAndUnknownEmail`
asserts byte-for-byte identical wire envelopes for matched +
unmatched + mail-failed branches.
- `web/tests/api/__snapshots__/auth/lost_password_generic.json`
locks the one canonical envelope; the obsolete
`lost_password_not_registered.json` and `lost_password_mail_failed.json`
snapshots are deleted.
- `web/tests/e2e/specs/flows/lostpassword-toast.spec.ts` adds
two chrome-side tests that drive the form submit end-to-end
for known + unknown emails and assert the painted toast is
identical. The file is now `test.describe.configure({ mode:
'serial' })` because the existing marquee #1403 test seeds a
known `:prefix_admins.validate` token that the new tests
would race under default `workers > 1`.
Closes #1456.
S1 (AGENTS.md sibling-leak description): rewrite the api_auth_login
follow-up note to accurately describe the 1-request-per-username
enumeration oracle (empty-password short-circuit at line 50-52 runs
BEFORE NormalAuthHandler so `attempts` is never incremented; lockout-
after-5 gate provides no protection against this surface).
S2 (AGENTS.md audit-log discipline): document the residual log-DoS
risk on the matched-branch SMTP-failure path. An attacker who knows
one registered email + catches SMTP broken can flood `:prefix_log`
at request rate AND roll the legitimate user's outstanding `validate`
token on every request. Closing requires a per-(IP x email) rate
limiter the panel doesn't have; documented as a tracking follow-up.
S3 (disabled-branch test): pin that `?config.enablenormallogin=0`
returns a byte-identical envelope for matched + unmatched emails.
The handler returns ApiError('disabled', ...) on this branch — same
value for every caller, so the error envelope is acceptable per the
response-uniformity contract — but a future regression that adds a
per-account suffix would silently re-open enumeration.
S4 (strengthen testLostPasswordRollsValidateTokenForKnownEmail):
assert Mail::send was actually REACHED on the match branch by
checking both the handler's "Password reset mail failed" log entry
AND Mail::send's own "Mail not configured" entry land in
`:prefix_log` after the call. Catches a regression that skips the
send attempt while still rolling the token + returning the generic
envelope — the real reset flow would silently never email anyone.
N1 (OWASP citation fix): replace "OWASP ASVS v4 §3.2.1" (which
addresses session-token confidentiality, not credential recovery)
with "OWASP ASVS v4 Credential Recovery §V2.5". Also expand the
framework list to include GitHub's password-recovery defaults
alongside Django + Rails.
N2 (neutral body copy): change "If that email is registered to an
admin account on this panel" to "If an account is registered to
that email address". The panel URL + form heading already advertise
the surface; the response copy should not narrow the scope
further. Mirrors GitHub / Django / Rails defaults. Updated the
snapshot + the E2E spec's text-content assertion to match.
E2E flake mitigation: the marquee #1403 happy-path test seeds
`admin@example.test`'s validate column then GETs a URL keyed on it.
Pre-fix the form-POST tests targeted the same row, racing it across
chromium ⇄ mobile-chromium projects (test.describe.configure({
mode: 'serial' }) is within-project; CI's `workers: 1` masks the
race). Lift the form-POST tests to a dedicated `lostpw-enum-known@
example.test` admin row seeded idempotently via the new
`seed-lostpassword-enum-admin-e2e.php` shim + `seedLostpassword
EnumAdminE2e` helper in `fixtures/db.ts`. The marquee test's pre-
existing latent race (not introduced by #1456) is unaffected by
this PR; CI passes deterministically and per-project local runs
pass deterministically.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #1456. The Forgot Password endpoint (
auth.lost_password) nolonger reveals whether the submitted email is registered.
Check E-Mailsuccessenvelope; an unregistered email returned
error: not_registeredwith the message "The email address you supplied is not registered
on the system"; SMTP failure returned
error: mail_failed. Anyanonymous visitor could enumerate registered admin accounts one
request at a time.
— returns the same generic envelope:
{ok: true, data: {message: {title: 'Check E-Mail', body: 'If an account is registered to that email address, a password reset link has been sent. Please check your inbox (and your spam folder).', kind: 'blue'}}}. The DBwrite + SMTP round-trip still only run when the row matches; the
miss branch returns the envelope without touching
:prefix_adminsor the mailer (so the handler is NOT an open-relay vector).
The fix follows the OWASP Forgot Password Cheat Sheet's "Return a
consistent message" guidance and mirrors Django's
password_reset,Rails's
devise/recoverable, and GitHub's password-recoverydefaults.
Why not just hide the error message?
The wire shape was the oracle, not just the rendered toast. Even
without rendering, a curl-driven attacker could read the JSON
envelope's
error.code(not_registeredvsmail_failedvsok: true) to enumerate. The fix collapses all three branches to asingle envelope shape so the wire layer no longer signals.
Sibling auth surfaces (audit findings)
api_account_change_emailis gated to authenticated owners andreturns a generic
validationerror foremail_taken— noper-target leak.
api_auth_loginHAS a sibling enumeration oracle (empty-passwordshort-circuit returns
?m=empty_pwdfor known users vs?m=failedfor unknown users, and the lockout-after-5 gate does NOT protect
against this because it fires downstream of the short-circuit).
Documented under "Public auth surfaces: response-shape uniformity"
in AGENTS.md as a tracking follow-up — out of scope for Forgot Password - Leaks if email address is valid or not #1456
because it's a different code path with its own UX implications
(the login form's error copy needs a parallel pass).
Residual timing / log-DoS surface (documented, not closed)
round-trip; the unmatched branch does 1 DB query and returns
immediately. A network-side attacker can statistically distinguish
the two with enough samples. Closing requires either constant-time
execution (always send a real mail, e.g. to a sink address, on
the miss branch — wasteful + spam-vector) or a per-IP rate limit
the panel doesn't currently have. Acknowledged in the handler
docblock + AGENTS.md.
a single registered email AND catches the panel with broken SMTP
can hammer the endpoint to flood
:prefix_log(each failure logsan audit entry) AND roll the legitimate user's outstanding
validatetoken on every request. Closing requires a per-(IP ×email) rate limiter. Documented in AGENTS.md "Audit-log
discipline".
Neither is the user-enumeration vulnerability the issue calls out;
both are flagged for a follow-up.
Audit logging
Log::add(LogType::Error, 'Password reset mail failed', …)entry so operators can diagnose SMTPmisconfiguration.
Mail::sendalready logs the underlyingtransport exception ("Mail not configured" / "Mail error" /
"Mail send failed"); the handler adds a paired entry that pins
the action that triggered it.
anonymous caller write to
:prefix_logat request rate (DoSamplification) and (2) be a side-channel into "this address is
unknown" visible to anyone who reads the audit log.
Files
web/api/handlers/auth.php— collapse three branches to onegeneric envelope via
_api_auth_lost_password_generic_response().Single source for the wire shape; load-bearing copy.
web/tests/api/AuthTest.php— 6 new tests pin the contract:identical envelope across known + unknown emails (the marquee
contract), no DB touch on the miss branch, validate-token roll
on the match branch, Mail::send actually reached on the match
branch (verified via audit-log entries), and the
?config.enablenormallogin=0branch returns a byte-identicalenvelope regardless of whether the email matched.
web/tests/api/__snapshots__/auth/lost_password_generic.json—new snapshot for the generic envelope. The pre-fix
lost_password_not_registered.json+lost_password_mail_failed.jsonsnapshots are deleted.
web/tests/e2e/specs/flows/lostpassword-toast.spec.ts— 2 newend-to-end tests drive the form submission for known + unknown
emails and assert the SAME generic toast paints. Uses a
dedicated
lostpw-enum-known@example.testadmin row (seededidempotently via the new shim) so the
validate-token UPDATEdoesn't race the marquee Audit follow-up: silent <script>ShowBox(...)</script> blobs lose user feedback on lostpassword / protest / banlist / commslist / admin.edit.comms #1403 happy-path test across Playwright
projects.
web/tests/e2e/scripts/seed-lostpassword-enum-admin-e2e.php+web/tests/e2e/fixtures/db.ts— new shim + TS wrapper for thededicated admin row.
AGENTS.md— new "Public auth surfaces: response-shapeuniformity" convention block, matching Anti-patterns entry,
"Where to find what" row, and the sibling
api_auth_login+log-DoS follow-up notes.
web/pages/page.lostpassword.php— comment clarification only;the page handler still surfaces the disabled-form fallback and
the email-shape inline error, which are NOT enumeration channels.
web/scripts/api-contract.js— regenerated to pick up updatedhandler docblock.
Test plan
(11 AuthTest cases, including 6 new ones pinning the
enumeration-prevention contract)
--workers=1(mirrors CI behaviour per AGENTS.md "Playwright E2E specifics")
auth.lost_passwordwith an unknown email gets the same envelope as a hit on a
registered email (admin@example.test)
(S1-S4) + 2 nits (N1-N2)